-
-
Notifications
You must be signed in to change notification settings - Fork 205
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes: Identifying Users with OpenTelemetry doesn't work #2618
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work! Happy you found a way around without having to introduce a new package or add AspNetCore as a dep to Sentry.OTel!!
Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>
Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>
Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>
Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic!
Co-authored-by: Stefan Jandl <reg@bitfox.at>
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
- Identifying Users with OpenTelemetry doesn't work ([#2618](https://github.com/getsentry/sentry-dotnet/pull/2618)) If none of the above apply, you can opt out of this check by adding |
Hey @yudielcurbelo,
So you shouldn't need to use this directly. You'd use whatever Authorization scheme you're using already and Sentry will be able to pull details for the user from the HttpContext. |
How should we rewrite the example stated here: https://docs.sentry.io/platforms/dotnet/guides/aspnetcore/#capturing-the-affected-user
|
Ah, I see. You want to create your own User Factory that gets used by the OpenTelemetry integration? In that case you're right... we'd need to make Thanks for calling this out. I've created a new ticket to track this: #2714 |
@@ -0,0 +1,6 @@ | |||
namespace Sentry; | |||
|
|||
internal interface ISentryUserFactory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jamescrosswell I'm hitting the IUserFactory warning in our builds but when I went to do the cutover to ISentryUserFactory to silence them I discovered this interface is marked internal so I can't implement it in my projects. Was this unintentional? If intentional, how are we supposed to proceed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jeremy-allocate , see #2618 (comment) above.
If it's an emergency and you just need to get a build out, you can disable the warning with a pragma, as we've done here:
sentry-dotnet/src/Sentry.AspNetCore/Extensions/DependencyInjection/ServiceCollectionExtensions.cs
Lines 26 to 28 in bbe4fa9
#pragma warning disable CS0618 | |
services.TryAddSingleton<IUserFactory, DefaultUserFactory>(); | |
#pragma warning restore CS0618 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good, thanks @jamescrosswell. We'll live with the warnings until #2714 is released.
As an FYI, we just made Release 3.40.1 that addresses this:
|
Awesome, many thanks! |
Fixes Identifying Users with OpenTelemetry doesn't work #2616.
Key implementation details
sentry-dotnet/src/Sentry.AspNetCore/Extensions/DependencyInjection/ServiceCollectionExtensions.cs
Line 25 in ad6968c
ISentryUserFactory
to theDefaultUserFactory
(eventually this should replaceIUserFactory
). This enables us to get the user data without having to add any dependencies to theSentry.OpenTelemetry
package. See:sentry-dotnet/src/Sentry.AspNetCore/IUserFactory.cs
Lines 10 to 11 in ad6968c
AspNetCoreEnricher
:sentry-dotnet/src/Sentry.OpenTelemetry/TracerProviderBuilderExtensions.cs
Lines 37 to 42 in ad6968c